Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow mutable-for-identity-only types to be inlined into IR #54034

Merged
merged 2 commits into from
Apr 11, 2024
Merged

Allow mutable-for-identity-only types to be inlined into IR #54034

merged 2 commits into from
Apr 11, 2024

Conversation

Keno
Copy link
Member

@Keno Keno commented Apr 11, 2024

This extends the value-inability predicate to allow mutable structs that do not have any mutable fields. The motivating example for this is ScopedValue, which is mutable only for identity. By allowing it in this predicate, more of the ScopedValue support code becomes concrete-eval eligible, rather than attempting to constprop for every single ScopedValue, saving compilation time.

@Keno Keno requested a review from aviatesk April 11, 2024 04:28
@Keno @aviatesk
Allow mutable-for-identity-only types to be inlined into IR
a643984 
This extends the value-inability predicate to allow mutable structs
that do not have any mutable fields. The motivating example for this
is ScopedValue, which is mutable only for identity. By allowing it
in this predicate, more of the ScopedValue support code becomes
concrete-eval eligible, rather than attempting to constprop for
every single ScopedValue, saving compilation time.
@aviatesk
Copy link
Member

I think this PR will make constant ScopedValue inlined into the IR, but I'm not sure how that's supposed to improve the concrete eval eligibility. For instance, I doubt that getindex(::ScopedValue) will be eligible for concrete evaluation even with this PR.

@Keno
Copy link
Member Author

Keno commented Apr 11, 2024

The thing that no longer gets constproped is the call Pair(::ScopedValue, val) (as in with(sc => val), which was also concrete-eval'd before, but we were throwing away the result and doing constprop anyway, because a pair with a ScopedValue in it failed this predicate.

@aviatesk
Copy link
Member

I see. I will add simple test case for that.

@aviatesk aviatesk force-pushed the kf/mutablesizepred branch from 2b441a7 to 2118d11 Compare April 11, 2024 06:54
@aviatesk aviatesk added the merge me PR is reviewed. Merge when all tests are passing label Apr 11, 2024
@aviatesk aviatesk merged commit a7fd6a7 into master Apr 11, 2024
8 checks passed
@aviatesk aviatesk deleted the kf/mutablesizepred branch April 11, 2024 12:05
@fatteneder fatteneder removed the merge me PR is reviewed. Merge when all tests are passing label Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aviatesk aviatesk aviatesk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Keno @aviatesk @fatteneder